-
Notifications
You must be signed in to change notification settings - Fork 385
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: refactor implementation and design smells #1088
Conversation
… and structure modules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Please see some comments below
@@ -104,7 +100,8 @@ public static QsAlignResult align(List<Subunit> s1, List<Subunit> s2, | |||
continue; | |||
|
|||
// Use structural alignment to match the subunit clusters | |||
if (c1.get(i).mergeStructure(c2.get(j),cParams)) { | |||
SubunitClusterMerge subunitClusterMerge = new SubunitClusterMerge(c1.get(i)); | |||
if (subunitClusterMerge.mergeStructure(c2.get(j),cParams)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion it was more readable before, as you could see the i/j merging in a single line. Would you have a strong reason for this change?
@@ -38,7 +39,7 @@ public class PdbPairsMessage { | |||
|
|||
public PdbPairsMessage(){ | |||
|
|||
method = PdbPairXMLConverter.DEFAULT_METHOD_NAME; | |||
method = FatCatRigid.algorithmName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was intended to point to DEFAULT_METHOD_NAME
. Then there's a constant that's specific for the default.
if (params.isUseEntityIdForSeqIdentityDetermination() && | ||
clusters.get(c1).mergeIdenticalByEntityId(clusters.get(c2))) { | ||
subunitClusterMerge.mergeIdenticalByEntityId(clusters.get(c2))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have the same comment here as above. I don't see this improves readability. And same for a few other cases below.
* i.e. it is between symmetry-related molecules (with same chain identifier) | ||
* @return | ||
*/ | ||
public boolean isSymRelated() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Pair class is general and intended for any kind of pair (note the generic type T
). This would break that generality.
This is a fix for several code smells discovered in the code.
Tests are passing for all fixes. Test class TestSubunitClusterMerge was created for the new extracted class mentioned in #4.